Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tarantool binary protocol support #1023

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

0x501D
Copy link

@0x501D 0x501D commented Dec 15, 2022

Added Tarantool binary protocol parsing (IPROTO).

Protocol info From IANA database:
Service Name: tarantool
Port Number: 3301
Transport Protocol: tcp/udp
Description: Tarantool in-memory computing platform

@0x501D 0x501D marked this pull request as draft December 15, 2022 17:34
@0x501D 0x501D force-pushed the tarantool_proto branch 6 times, most recently from c07b9fe to 0268ccd Compare December 15, 2022 21:57
@guyharris
Copy link
Member

This isn't compiling with Visual Studio 2015; does it require a later version of Visual Studio?

@0x501D
Copy link
Author

0x501D commented Dec 16, 2022

This isn't compiling with Visual Studio 2015; does it require a later version of Visual Studio?

Should not require, I'm now dealing with all the fails.

@0x501D 0x501D force-pushed the tarantool_proto branch 4 times, most recently from b0e124c to 4d666ec Compare December 16, 2022 15:07
@0x501D
Copy link
Author

0x501D commented Dec 16, 2022

This isn't compiling with Visual Studio 2015; does it require a later version of Visual Studio?

Yes, MSVC 2015 is too old to compile the msgpuck library. Is this a critical issue?
I can make support for the tarantool protocol as a feature and link to msgpuck, but this library is not widely distributed across Linux distributions and other operating systems. That's why I wanted to include the library in tcpdump

@fxlb
Copy link
Member

fxlb commented Dec 16, 2022

Yes, MSVC 2015 is too old to compile the msgpuck library. Is this a critical issue?

Can you explain what the problems are with this version?

@0x501D
Copy link
Author

0x501D commented Dec 16, 2022

MSVC 2015

Yes, MSVC 2015 is too old to compile the msgpuck library. Is this a critical issue?

Can you explain what the problems are with this version?

Unfortunately, I don't have an operating system where I can debug a build with MSVC. I need more time to understand what the problem is in the blind.

@fxlb
Copy link
Member

fxlb commented Dec 16, 2022

(Because MSVC 2015 is able to build tcpdump since 2018.)

@fxlb
Copy link
Member

fxlb commented Dec 16, 2022

Please avoid non ASCII code and output:

$ git show|grep -P '[\x7F-\xFF]' 
+#define __STDC_CONSTANT_MACROS 1 /* make С++ to be happy */
+#define __STDC_LIMIT_MACROS 1    /* make С++ to be happy */
+TUPLE: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]
+TUPLE: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]
+body: {35: "replicator", 33: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]}

@fxlb
Copy link
Member

fxlb commented Dec 17, 2022

but this library is not widely distributed across Linux distributions and other operating systems

Are you sure?
https://repology.org/project/msgpuck/packages

@fxlb
Copy link
Member

fxlb commented Dec 17, 2022

Or another library?

@0x501D
Copy link
Author

0x501D commented Dec 17, 2022

Or another library?

Apparently the library is already quite well distributed, I will remake the PR to an external dependency.

Though I don’t really understand how to be then with CI and a new dependency. All tests will definitely fail.

@fxlb
Copy link
Member

fxlb commented Dec 19, 2022

Though I don’t really understand how to be then with CI and a new dependency. All tests will definitely fail.

There can be conditional tests. See e.g. tests/crypto.tests.

@0x501D 0x501D force-pushed the tarantool_proto branch 6 times, most recently from 53349bd to 58e6e7f Compare December 21, 2022 08:42
@0x501D
Copy link
Author

0x501D commented Dec 21, 2022

Please avoid non ASCII code and output:

$ git show|grep -P '[\x7F-\xFF]' 
+#define __STDC_CONSTANT_MACROS 1 /* make С++ to be happy */
+#define __STDC_LIMIT_MACROS 1    /* make С++ to be happy */
+TUPLE: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]
+TUPLE: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]
+body: {35: "replicator", 33: ["chap-sha1", "Ë\u0001\u0001È\"Fêèu\u0011)-}À»\u001bà"]}

Fixed. Added base64 encoding of binary data to the msgpuck library.

@0x501D
Copy link
Author

0x501D commented Dec 21, 2022

This isn't compiling with Visual Studio 2015; does it require a later version of Visual Studio?

Fixed. The msgpuck library contained a bug in the code that caused it to fail to build on MSVC.

@0x501D 0x501D marked this pull request as ready for review December 21, 2022 08:46
@0x501D
Copy link
Author

0x501D commented Mar 20, 2023

@fxlb, Is there anything else I need to fix?

@fxlb
Copy link
Member

fxlb commented Aug 9, 2023

Apparently the library is already quite well distributed, I will remake the PR to an external dependency.

I don't see the update. Do you have pushed it?

Another important point:
Your code give many problems like SEGV, heap-buffer-overflow, heap-use-after-free, etc. when fuzzing.
One of the reasons is that you don't use GET_*() or ND_TCHECK_*() macros.
See CONTRIBUTING.md, particularly part "Code style and generic remarks" items 5. and 6.

@0x501D
Copy link
Author

0x501D commented Aug 9, 2023

I don't see the update. Do you have pushed it?

Thanks for the answer. I had to leave the built-in version of msgpuck since upstream prints binary data as is. In the built-in version, I fixed it:

// Fixed. Added base64 encoding of binary data to the msgpuck library.

With regards to the rest - proceeded to correct.

@fxlb
Copy link
Member

fxlb commented Aug 9, 2023

It's not the goal of tcpdump to add/maintain the source code of an external library.
Thus in this PR msgpuck.c, msgpuck.h and msgpuck_hints.c should be removed.

For libmsgpuck-dev, it's like libsmi2-dev in tcpdump. we need to link to it and use it if the configure/cmake finds it.

The Tarantool dissector will be present if libmsgpuck-dev will be found on the system where the build is being done.

@0x501D
Copy link
Author

0x501D commented Aug 9, 2023

It's not the goal of tcpdump to add/maintain the source code of an external library. Thus in this PR msgpuck.c, msgpuck.h and msgpuck_hints.c should be removed.

For libmsgpuck-dev, it's like libsmi2-dev in tcpdump. we need to link to it and use it if the configure/cmake finds it.

The Tarantool dissector will be present if libmsgpuck-dev will be found on the system where the build is being done.

Understood. I will remove msgpuck from the tcpdump source code and provide a workaround to print binary data.

@0x501D 0x501D force-pushed the tarantool_proto branch 3 times, most recently from 876db26 to a3850fd Compare August 21, 2023 11:01
@0x501D
Copy link
Author

0x501D commented Aug 30, 2023

Hello, pull request updated:

  • msgpuck is added as an external dependency.
  • Added validation of the iproto protocol. The check from the library is used. mp_check_* functions.
  • Added correct handling of multiple iproto messages within one packet.
  • Added a test to check the correctness of parsing truncated packets.

@0x501D 0x501D force-pushed the tarantool_proto branch 2 times, most recently from 9561bd7 to 0337cf4 Compare August 30, 2023 12:56
@0x501D
Copy link
Author

0x501D commented Aug 30, 2023

Something wrong with linux-amd64 test workflow. It fails with Timed out error.
@fxlb can you tell me if this is a PR or CI problem?

Update: problem with CI is gone.

Added Tarantool binary protocol parsing (IPROTO).

Protocol info From IANA database:
Service Name: tarantool
Port Number: 3301
Transport Protocol: tcp/udp
Description: Tarantool in-memory computing platform
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants